Update Cppyy::GetScope to handle nested namespaces and templates
#116
Conversation
clingwrapper/src/clingwrapper.cxx
Outdated
| if (!scope && (parent_scope == nullptr || parent_scope == Cpp::GetGlobalScope())) | ||
| scope = Cpp::GetScopeFromCompleteName(name); |
There was a problem hiding this comment.
I was thinking about updating
TCppScope_t Cpp::GetScopeFromCompleteName(const std::string &name)to
TCppScope_t Cpp::GetScopeFromCompleteName(const std::string &name, TCppScope_t parent)at CppInterOp.
Any suggestions?
There was a problem hiding this comment.
Yes, but I think the reason for this interface was to provide the fully scoped name. Do we benefit from providing the parent scope too? (vs using Cpp::GetScope(const std::string &name, TCppScope_t parent))
There was a problem hiding this comment.
Cpp::GetScope does not handle namespaces, like my_namespace::my_class.
And Cpp::GetScopeFromCompleteName does not have the parent scope argument.
The code here will be simplified. Thats a benefit.
|
Looks good! We should probably prevent the seg fault on the xfailing test though.. does it come from the scope lookup or the template instantiation? Maybe a case we can address in clingwrapper/ or whatever calls |
I suspect it might be after the call to |
If its after then I guess we are returning the wrong scope |
|
Hi @aaronj0. I investigated the failing test. namespace __cppyy_internal {
void init_PreIncrement__Iterator(PreIncrement::Iterator*& self, const Token& current) {
self = new PreIncrement::Iterator{current};
} }The stack trace goes through Pythonize.cxx:1779. If I run that single test standalone, it passes. Performing With pytest: Running standalone: You can see that the second ping @vgvassilev EDITED |
That needs to be fixed at CppInterOp reference: look at comments at compiler-research/cppyy-backend#116
|
I think it is fine to make it from xfail a crash and investigate later. |
clingwrapper/src/clingwrapper.cxx
Outdated
| Cppyy::TCppScope_t scope = Cpp::GetScope(name, parent_scope); | ||
| if (!scope && (parent_scope == nullptr || parent_scope == Cpp::GetGlobalScope())) | ||
| scope = Cpp::GetScopeFromCompleteName(name); | ||
| if (scope) | ||
| return scope; |
There was a problem hiding this comment.
| Cppyy::TCppScope_t scope = Cpp::GetScope(name, parent_scope); | |
| if (!scope && (parent_scope == nullptr || parent_scope == Cpp::GetGlobalScope())) | |
| scope = Cpp::GetScopeFromCompleteName(name); | |
| if (scope) | |
| return scope; | |
| if (Cppyy::TCppScope_t scope = Cpp::GetScope(name, parent_scope)) | |
| return scope; | |
| if (!parent_scope || parent_scope == Cpp::GetGlobalScope()) | |
| if (Cppyy::TCppScope_t scope = Cpp::GetScopeFromCompleteName(name)) | |
| return scope; |
You can simplify the code using that pattern...
Vipul-Cariappa
force-pushed
the
dev/update-GetScope
branch
from
6a9ae67 to
ae8c51a
Compare
| Cppyy::TCppScope_t scope = Cpp::GetScope(pure_name, parent_scope); | ||
| if (!scope && (!parent_scope || parent_scope == Cpp::GetGlobalScope())) | ||
| scope = Cpp::GetScopeFromCompleteName(pure_name); |
There was a problem hiding this comment.
| Cppyy::TCppScope_t scope = Cpp::GetScope(pure_name, parent_scope); | |
| if (!scope && (!parent_scope || parent_scope == Cpp::GetGlobalScope())) | |
| scope = Cpp::GetScopeFromCompleteName(pure_name); | |
| Cppyy::TCppScope_t scope = Cpp::GetScope(pure_name, parent_scope) | |
| if (scope) | |
| return scope; | |
| if (!parent_scope || parent_scope == Cpp::GetGlobalScope()) | |
| scope = Cpp::GetScopeFromCompleteName(pure_name); |
There was a problem hiding this comment.
We are not returning the scope here. We are using scope to InstantiateTemplate and returning the template.
Vipul-Cariappa
force-pushed
the
dev/update-GetScope
branch
from
ae8c51a to
765c41d
Compare
That needs to be fixed at CppInterOp reference: look at comments at compiler-research/cppyy-backend#116
11 new tests pass.
1 xfail, segfaults with this change.